AROSLSRE-1158: add --install-scope flag to hypershift install#8725
AROSLSRE-1158: add --install-scope flag to hypershift install#8725shubhadapaithankar wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@shubhadapaithankar: This pull request references AROSLSRE-1158 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Please specify an area label DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: shubhadapaithankar The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds an InstallScope string to Options, validates it to allow only "", "all", "crds", or "resources", wires a new --install-scope CLI flag defaulting to "all", and changes InstallHyperShiftOperator to conditionally perform CRD dry-run/coordination/application and wait logic only when scope includes CRDs, and to apply operator resources and wait for deployment availability only when scope includes resources. It also adds Outputs.IsValid(), IncludesCRDs(), and IncludesResources(), removes the sets import, and updates ValidateRender to use Outputs.IsValid(). Sequence Diagram(s)sequenceDiagram
participant CLI
participant InstallHyperShiftOperator
participant ClusterCAPI
participant KubernetesAPI
participant Deployment
CLI->>InstallHyperShiftOperator: call with InstallScope
alt InstallScope == "all" or "crds"
InstallHyperShiftOperator->>KubernetesAPI: perform CRD dry-run validation
InstallHyperShiftOperator->>ClusterCAPI: detect/coordinate Cluster CAPI (unmanaged CRDs)
InstallHyperShiftOperator->>KubernetesAPI: apply CRDs
InstallHyperShiftOperator->>KubernetesAPI: waitUntilEstablished
end
alt InstallScope == "all" or "resources"
InstallHyperShiftOperator->>KubernetesAPI: apply operator resources
InstallHyperShiftOperator->>Deployment: WaitUntilAvailable
end
🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
fe80425 to
ed5f298
Compare
|
/area hypershift-operator |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8725 +/- ##
==========================================
+ Coverage 41.59% 41.67% +0.08%
==========================================
Files 758 758
Lines 93925 93949 +24
==========================================
+ Hits 39066 39152 +86
+ Misses 52113 52049 -64
- Partials 2746 2748 +2
... and 1 file with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ed5f298 to
ff45f66
Compare
celebdor
left a comment
There was a problem hiding this comment.
Thanks for the PR! The approach makes sense given the container constraints (no kubectl available in the Helm hook image).
The hypershift install render subcommand already defines an Outputs type with OutputAll, OutputCRDs, and OutputResources constants, along with validation logic in ValidateRender(). This PR duplicates those values as raw strings in both the validation switch and the scope conditionals.
I'd suggest reusing the existing type by adding methods to it:
func (o Outputs) IsValid() bool {
switch o {
case OutputAll, OutputCRDs, OutputResources:
return true
default:
return false
}
}
func (o Outputs) IncludesCRDs() bool {
return o == OutputAll || o == OutputCRDs
}
func (o Outputs) IncludesResources() bool {
return o == OutputAll || o == OutputResources
}Then the install path becomes:
scope := Outputs(opts.InstallScope)
if scope == "" {
scope = OutputAll
}
if scope.IncludesCRDs() {
// ...
}
if scope.IncludesResources() {
// ...
}And the validation simplifies to:
if o.InstallScope != "" && !Outputs(o.InstallScope).IsValid() {
errs = append(errs, fmt.Errorf("invalid --install-scope value %q: must be '%s', '%s', or '%s'", o.InstallScope, OutputAll, OutputCRDs, OutputResources))
}This also benefits the render path — its switch and validation can use the same methods, and the sets import can be dropped.
Additionally, the --outputs scope filtering on the render side has no test coverage today either. Since both paths would now share the same methods, adding a test that calls RenderHyperShiftOperator with each scope value and asserts CRDs vs resources in the output (using *apiextensionsv1.CustomResourceDefinition type checks) would cover the logic for both commands. I prototyped this locally — happy to share the test if helpful.
|
I prototyped the aforementioned refactor and tests at |
ff45f66 to
90f713b
Compare
Add a new --install-scope flag to the hypershift install command that controls which subset of manifests are applied: - "all" (default): installs CRDs and resources (existing behavior) - "crds": installs only CRDs - "resources": installs only resources (operator deployment and RBAC) Reuses the existing Outputs type from the render subcommand, adding IsValid(), IncludesCRDs(), and IncludesResources() helper methods. Both the install and render paths now use the shared methods, and the render path's switch statement is replaced with the same conditional pattern. The sets import is dropped. Includes unit tests for validation (valid/invalid scope values) and render output filtering (verifying CRDs vs resources in output). This enables consumers to split the hypershift install into phases, allowing CRD-dependent manifests to be applied between the CRD installation and operator startup. Ref: https://redhat.atlassian.net/browse/AROSLSRE-1158 Co-authored-by: celebdor <[email protected]> Co-authored-by: Cursor <[email protected]>
90f713b to
67b79f6
Compare
|
Thanks @celebdor for the review and the prototype! Addressed all feedback:
|
|
@shubhadapaithankar: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Confirmed. Now I have all the evidence needed. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe codecov/patch check failed because only 33.33% of the new/changed executable lines introduced by this PR are covered by tests, falling short of the 41.59% minimum patch coverage threshold. The coverage gap is caused entirely by the new scope-based conditional logic added inside Root CauseThe PR adds ~32 new executable lines inside
The ~15 covered lines come from Recommendations
Evidence
|
|
@Nirshal @sdminonne friendly ping - could you take a look when you get a chance? Small change adding |
What this PR does / why we need it
Adds a new
--install-scopeflag to thehypershift installcommand that controls which subset of manifests are applied:all(default): installs CRDs and resources (existing behavior, fully backwards compatible)crds: installs only CRDsresources: installs only resources (operator deployment and RBAC)Why we need it
In ARO-HCP, the HyperShift Helm chart uses post-install hooks to run
hypershift install. There's a race condition where the operator starts and creates aClusterSizingConfigurationbefore the Helm hook can apply its own version, causing apply conflicts.Splitting the install into phases allows:
--install-scope=crds)ClusterSizingConfiguration) to be applied by Helm--install-scope=resources)Which issue(s) this PR fixes
Fixes: https://redhat.atlassian.net/browse/AROSLSRE-1158
Related: https://redhat.atlassian.net/browse/AROSLSRE-313
Checklist
Summary by CodeRabbit
New Features
Refactor
Tests